Correct check amount when printed from subaccount #1811
Conversation
There was a problem hiding this comment.
This change doesn't add an option to the dialog, it just totals up the splits in the account's sub accounts if the register is opened with the sub accounts. That's fine, it's intuitively the right thing to do. Please change the commit message to say so.
The check's amount when printing a Find-results dialog in the presence of sub accounts is a bit random, you might take a look at that.
A companion PR to gnucash-docs to update the manual section 6.15 would be nice.
gnucash/gnome/dialog-print-check.c
Outdated
| if (pcd->account) | ||
| { | ||
| /* A parent account, e.g. from a subaccount register plugin page. | ||
| * Subtotal the amount of all splits from children accounts. */ | ||
| GList *children_accounts = NULL; | ||
| GList *node, *child; | ||
| children_accounts = gnc_account_get_descendants(pcd->account); | ||
| children_accounts = g_list_append(children_accounts, pcd->account); | ||
| amount = gnc_numeric_zero(); | ||
|
|
||
| for (node = xaccTransGetSplitList(trans); node; node = node->next) | ||
| { | ||
| Split *split = node->data; | ||
| Account* acct = xaccSplitGetAccount(split); | ||
| for (child = children_accounts; child; child = child->next) | ||
| { | ||
| if (acct == child->data) | ||
| { | ||
| amount = gnc_numeric_add_fixed(amount, xaccSplitGetAmount(split)); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| g_list_free(children_accounts); | ||
| children_accounts = NULL; | ||
| } | ||
| else | ||
| { | ||
| /* Print just the amount of the split. */ | ||
| amount = xaccSplitGetAmount(pcd->split); | ||
| } | ||
| amount = gnc_numeric_abs(amount); |
There was a problem hiding this comment.
The computation should be a separate function so that draw_page_item calls amount = compute_amount(pcd);.
gnucash/gnome/dialog-print-check.c
Outdated
| for (child = children_accounts; child; child = child->next) | ||
| { | ||
| if (acct == child->data) | ||
| { | ||
| amount = gnc_numeric_add_fixed(amount, xaccSplitGetAmount(split)); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't loop on all of pcd->account's children, use
if (pcd->account == xaccSplitGetAccount(split) ||
pcd->account == gnc_account_get_parent(xaccSplitGetAccount(split)))
amount = gnc_numeric_add_fixed(amount, xaccSplitGetAmount(split));
There was a problem hiding this comment.
I have like 5 levels of subaccounts... won't gnc_account_get_parent return only the direct parent?
There was a problem hiding this comment.
Not pointing fingers, but I copied this code from the existing ledger display logic:
gnucash/gnucash/register/ledger-core/split-register-model.c
Lines 112 to 126 in 4da655d
There was a problem hiding this comment.
So? There's lots of crappy code in GnuCash. That's no excuse for adding more.
There was a problem hiding this comment.
I have like 5 levels of subaccounts... won't
gnc_account_get_parentreturn only the direct parent?
Yup. But gnc_account_get_children also goes only one level. You'd need to use gnc_account_get_descendants or recurse on gnc_account_get_children, which is what get_descendants does under the hood. Either gets ugly so you might consider gnc_account_foreach_descendant with a test-and-accumulate function that checks each account in turn for splits in the transaction rather than the other way around. That way you're doing the more expensive loop only once instead of once per split.
There was a problem hiding this comment.
Sorry, not being defensive! I'm happy to use whatever best practices you'd prefer. I had simply found some existing code that did what I needed and copied it. However, both do use gnc_account_get_descendants to get all children accounts recursively into a list...
you might consider
gnc_account_foreach_descendantwith a test-and-accumulate function that checks each account in turn for splits in the transaction rather than the other way around. That way you're doing the more expensive loop only once instead of once per split.
Done. Found a few other helpers so all manual loops are removed.
Previously, GnuCash would include only a single split's amount as the check amount. If a "bank" account has multiple subaccounts for budgeting reasons, and a transaction involves both "internal" (e.g. from one bank subaccount to another bank subaccount) and external transfers, then even when a "subaccount" register showing the top-level bank account is printed from, the check will have the amount of the first split rather than the total of all splits which are from subaccounts. This change communicates the parent account (when viewed from a "subccount" register) to the check printing dialog so that the check amount matches the amount displayed in the register.
mabrowning
force-pushed
the
check-print-subacct
branch
from
fd0a85a
to
c2ef684
Compare
Done. No more mention of options.
I had never personally printed a check from Find results, but yeah it warns about printing a check with splits from multiple accounts, but then seems to print the amount of the first split in the transaction. I'm not sure how best to identify the appropriate "parent" account in this case. For awhile I had a local change that found the first account underneath the account named I did consider adding an option to each account indicating if it is "check bearing" or something, and there is the "Account Type" with experimental support for "Checking Account" in the codebase, but that seemed like a wider change I wasn't ready to sign up for. |
|
Much nicer. Thanks! |
Previously, GnuCash would include only a single split's amount as the check amount. If a "bank" account has multiple subaccounts for budgeting reasons, and a transaction involves both "internal" (e.g. from one bank subaccount to another bank subaccount) and external transfers, then even when a "subaccount" register showing the top-level bank account is printed from, the check will have the amount of the first split rather than the total of all splits which are from subaccounts.
This change communicates the parent account (when viewed from a "subaccount" register) to the check printing dialog so that the check amount matches the amount displayed in the register.
e.g.
Before:
After:
